-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: experimental TypeSafeJSONModel #706
base: main
Are you sure you want to change the base?
Conversation
…rwrite to interface overwrite
@petermuessig could you please give me some feedback if you need anything else on this PR? |
Sorry, @Popescu-PfeifferMarc - I wanted to cross-check with @akudev - we will do so this week and keep you posted... |
we would suggest the following small adoptions: Location: TODO: check if inheriting from the One thing which may still be interesting is whether it should be rather consumed as And I would also prefer a small example in the We may consider to move it into the official type definitions of UI5. Would you still be interested in showing it in one of the next UI5ers live webinars? |
@petermuessig Thanks for your feedback 👍 I will work on these improvements. Next 3 weeks for me are very full due to university exams so it might take some time. UI5ers live webinar would be cool 👍 |
Hi @petermuessig , I am now back from my university exams and quick vacation. I've added a Readme.md file with some explanations on how to use the Typed JSONModel and updated the example app to use the Typed JSONModel. I also renamed the package as you suggested. I am not so sure about using the Typed JSONModel in the default UI5 type definitions, since it still has some caveats, such as not properly working with recursive types in all cases, not supporting deeply nested objects (default limit is 10 objects deep), as well as a couple other small things outlined in the Readme.md file. Furthermore I believe that the pattern of defining an empty model and then adding properties to that model one by one is pretty common. This won't work out of the box with the Typed JSONModel since TypeScript can't infer the type of the model from the default values, since it is empty by default. Developers can fix this by manually specifying the type of the model in the generic parameter, but this requires manual effort. Regarding the .d.ts file: As long as we aren't directly replacing the JSONModel type, I think .ts is the best option. This allows for incremental adoption across a codebase. Could you please take a look at the showcase and the Readme.md file and give me some feedback? |
Fixed commit message namings since commitizen didn't work for some reason
655ba74
to
9ff8023
Compare
No description provided.